-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Fix ICE when late-bound lifetimes don't appear in fn signature #147631
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
PR rust-lang#135479 changed how late-bound regions are registered for MIR borrowck, switching from `for_each_late_bound_region_in_item` to iterating over `bound_inputs_and_output.bound_vars()`. However, for regular functions, the latter only includes late-bound lifetimes that appear in the function's inputs or output, missing unused lifetime parameters. This caused an ICE when `replace_bound_regions_with_nll_infer_vars` tried to look up these unregistered regions. This ensures all late-bound regions are properly registered in the universal regions map before they're needed during region substitution.
|
This PR changes a file inside |
|
r? @davidtwco rustbot has assigned @davidtwco. Use |
|
|
r? types |
| | DefiningTy::Coroutine(..) | ||
| | DefiningTy::CoroutineClosure(..) => { | ||
| // For closures/coroutines, iterate over bound_vars to include implicit regions. | ||
| for (idx, bound_var) in bound_inputs_and_output.bound_vars().iter().enumerate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so. This seems incompatible with the closure_lifetime_binder feature?
I think we should get @lcnr's thoughts on this, on why this switch was made (and if it's appropriate to go back to calling for_each_late_bound_region_in_item on everything).
That doesn't seem to be quite true 🤔 from what I can tell all the ICE happen in cases where the fn-sig is malformed. The following compiles: struct S<'a, T: ?Sized>(&'a T);
fn b<'a>() -> S<'static, u32> {
S::<'a>(&0)
}
I would also expect the same to be true for closures. The #![feature(closure_lifetime_binder)]
struct S<'a, T: ?Sized>(&'a T);
fn b() {
for<'a> || -> S<'static, u32> {
S::<'a>(&0)
};
}I personally believe the bug here to be that we don't include late bound regions in the binder if there's an error. We could even add an assert in |
Iirc the main reason behind #135479 was that previously we lazily converted bound regions which feels questionable to me. I switched us to first add nll vars for every late-bound region, and then convert as normal, without having to optionally create new bound regions |
Okay, yes, that's actually what I would expect too! But wasn't quite sure. |
PR #135479 changed how late-bound regions are registered for MIR borrowck, switching from
for_each_late_bound_region_in_itemto iterating overbound_inputs_and_output.bound_vars(). However, for regular functions, the latter only includes late-bound lifetimes that appear in the function's inputs or output, missing unused lifetime parameters.This caused an ICE when
replace_bound_regions_with_nll_infer_varstried to look up these unregistered regions.This ensures all late-bound regions are properly registered in the universal regions map before they're needed during region substitution.
Fixes #135845